Highlight changed bytes in Frames view when Overwrite mode is enabled#708
Open
dontgetfoundout wants to merge 4 commits intocollin80:masterfrom
Open
Highlight changed bytes in Frames view when Overwrite mode is enabled#708dontgetfoundout wants to merge 4 commits intocollin80:masterfrom
dontgetfoundout wants to merge 4 commits intocollin80:masterfrom
Conversation
Author
|
Updated to add a setting to enable/disable this behavior:
|
edge90
reviewed
Sep 4, 2024
| const QModelIndex &index) const | ||
| { | ||
| const auto frame = index.data().value<CANFrame>(); | ||
| const unsigned char *data = reinterpret_cast<const unsigned char *>(frame.payload().constData()); |
There was a problem hiding this comment.
Isn't it better to continue to use QByteArray?
Comment on lines
+22
to
+25
| bool overwriteDups = model->getOverwriteMode(); | ||
| bool markChangedBytes = model->getMarkChangedBytes(); | ||
| int bytesPerLine = model->getBytesPerLine(); | ||
| bool useHexMode = model->getHexMode(); |
There was a problem hiding this comment.
Suggested change
| bool overwriteDups = model->getOverwriteMode(); | |
| bool markChangedBytes = model->getMarkChangedBytes(); | |
| int bytesPerLine = model->getBytesPerLine(); | |
| bool useHexMode = model->getHexMode(); | |
| const bool overwriteDups = model->getOverwriteMode(); | |
| const bool markChangedBytes = model->getMarkChangedBytes(); | |
| const int bytesPerLine = model->getBytesPerLine(); | |
| const bool useHexMode = model->getHexMode(); |
Comment on lines
+32
to
+33
| auto pen = painter->pen(); | ||
| const auto defaultColor = pen.color(); |
There was a problem hiding this comment.
Suggested change
| auto pen = painter->pen(); | |
| const auto defaultColor = pen.color(); | |
| const auto defaultColor = painter->pen().color(); |
Comment on lines
+39
to
+40
| int dataLen = frame.payload().count(); | ||
| if (dataLen < 0) dataLen = 0; |
There was a problem hiding this comment.
Suggested change
| int dataLen = frame.payload().count(); | |
| if (dataLen < 0) dataLen = 0; | |
| int dataLen = frame.payload().count(); | |
| if (dataLen < 0) dataLen = 0; | |
| const auto dataLen = std::max(frame.payload().count(), qsizetype{0}); |
edge90
reviewed
Sep 4, 2024
| return (double)timestamp / 1000.0; | ||
| break; | ||
| case TS_MICROS: | ||
| default: |
There was a problem hiding this comment.
is this really intentional? It should move to be the last entry if that's the case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
This change introduces a
QItemDelegatethat is used to paint the Data column of the main window's Frames view. Before the change, the value of the Data column was calculated as a string byCANFrameModel::data()and displayed using the defaultQStyledItemDelegate. In the current version of the changes, theCANFrameis returned as Data instead and the parsing and display logic has been moved toCanDataItemDelegate. Most of the structure of the code was maintained, but now it is orchestratingQPainterdraw calls rather than building up the string representation of the bytes.There are a lot of ways this solution could be tweaked (improved?). For example:
CANFrameModel::data()could parse the CANFrame into a streamlined data type with just the byte data, changed bytes, error message text, and some settings (like bits per line) and return that as aQVariantfor the delegate to display rather than this design which returnsQVariant::fromValue(CANFrame)and moves all of the parsing and display logic to the delegateCANFrameModelto read settings inCanDataItemDelegatebut this coupling could be reduced with another representationPreviously discussed as: #84 Color code changed bits in overwrite mode
While working on this, I found this already closed suggestion for the feature. I agree the sniffer window can be used to show this data, but I've come to like the visual feedback of having byte diffs in overwrite mode. Feel free to close this PR if you don't want to add this extra complexity to the project.
Screenshot
Example of the UI receiving Fuzzed frames using "Sweep" Bit Scanning
Validated:
Not tested:
Probably needs some updatesmaybe works after commit "Increase size of changed bytes bit field" still needs testingNotes:
It's been a little while since I've messed around with someone else's C++ codebase and I don't use it professionally atm. Please let me know if you have feedback (even if trivial).